-
-
Notifications
You must be signed in to change notification settings - Fork 873
Remove explicit names from initializers. #1654
Conversation
@@ -34,8 +34,7 @@ export function initialize(application) { | |||
}; | |||
|
|||
export default { | |||
name: 'shopping-cart', | |||
initialize: initialize | |||
initialize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would one-liner this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it like this because it's fairly common to add before
and after
and I wanted to maintain symmetry. I can change it though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to say you were right, it was a better idea to keep it multiline ;)
@@ -51,13 +50,12 @@ Let's add some simple logging to indicate that the instance has booted: | |||
|
|||
```app/instance-initializers/logger.js | |||
export function initialize(applicationInstance) { | |||
var logger = applicationInstance.lookup('logger:main'); | |||
let logger = applicationInstance.lookup('logger:main'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
logger.log('Hello from the instance initializer!'); | ||
} | ||
|
||
export default { | ||
name: 'logger', | ||
initialize: initialize | ||
initialize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one-liner as above?
@stefanpenner - Any objections? |
I'm cool with this change. |
☔ The latest upstream changes (presumably #1766) made this pull request unmergeable. Please resolve the merge conflicts. |
I don't have time to work on this right now, but I believe the maintainers have edit access for this branch. |
@@ -34,8 +34,7 @@ export function initialize(application) { | |||
}; | |||
|
|||
export default { | |||
name: 'shopping-cart', | |||
initialize: initialize | |||
initialize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to say you were right, it was a better idea to keep it multiline ;)
NOTE ONLY MERGE WHEN emberjs/ember.js#14338 IS IN A RELEASE |
This was released in [email protected] |
Since ember-load-initializers already adds names to initializers based on the filename, we should not bother specifying them in the initializer files themselves. This brings initializers more in-line with how helpers are handled. This commit also makes better use of ES2015 features. https://github.com/ember-cli/ember-load-initializers/blob/7ebe1d308cbbbfc3e6566cd05699e9a36fb7bddb/addon/index.js#L19-L20
Thanks @eventualbuddha, sorry for letting it slide for so long 😅 . |
Since ember-load-initializers already adds names to initializers based on the filename, we should not bother specifying them in the initializer files themselves. This brings initializers more in-line with how helpers are handled. This commit also makes better use of ES2015 features.